Automated Test: feature-clients-authz-implementation #313
Conversation
…valuation implementation for ClientsPermissionsV2 Closes #35564 Signed-off-by: Martin Kanis <mkanis@redhat.com>
📝 WalkthroughWalkthroughThis pull request introduces fine-grained authorization for Clients in Keycloak's admin permissions system. It adds a new CLIENTS resource type with scopes for configure, manage, view, and role-mapping operations, along with concrete permission evaluation logic and comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant TestCode as Test Code
participant PermEval as ClientPermissionEvaluator
participant PermV2 as ClientPermissionsV2
participant AuthzProvider as AuthorizationProvider
participant ResourceServer as ResourceServer
TestCode->>PermV2: canManage(ClientModel client)
PermV2->>PermV2: hasPermission(MANAGE, client)
PermV2->>AuthzProvider: evaluate(evaluationContext)
AuthzProvider->>ResourceServer: check permission against resource
ResourceServer-->>AuthzProvider: permission result
AuthzProvider-->>PermV2: evaluation result
PermV2-->>TestCode: boolean result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java`:
- Around line 95-98: Update the JavaDoc for the interface method
ClientPermissionEvaluator.requireView() to stop saying "Returns {`@code` true}"
and instead document that the method throws a ForbiddenException if {`@link`
`#canView`()} returns {`@code` false}; mirror the wording used by other requireXxx
methods (e.g., requireManage(), requireConfigure()) so the comment correctly
reflects the void signature and the thrown exception behavior.
- Around line 100-103: The JavaDoc for requireViewClientScopes() erroneously
says "Returns {`@code` true}" though the method is void; update its Javadoc to
match the semantics used in requireView() (e.g., state that the method enforces
the permission check and will throw an exception if the caller lacks permission
or simply requires view permissions) and remove any "Returns" phrasing so the
comment accurately describes the behavior of requireViewClientScopes().
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java`:
- Around line 173-178: The test uses a hardcoded index get(1) on
defaultClientScopes which risks IndexOutOfBounds; change the access to a safe
index or guard it: verify defaultClientScopes is non-empty and select an element
(e.g., defaultClientScopes.get(0)) or add an explicit bounds check before
calling clientResource.removeDefaultClientScope(clientScopeRep.getId()) and
clientResource.addOptionalClientScope(clientScopeRep.getId()) in
PermissionClientTest (referencing defaultClientScopes,
clientResource.removeDefaultClientScope, and
clientResource.addOptionalClientScope).
- Around line 319-322: The test currently calls
realmAdminClient.realm(realm.getName()).clients().create(null) which can trigger
server-side NPE/400; replace the null with a minimal valid ClientRepresentation
(populate required fields like clientId and any mandatory flags) and use that
when calling ClientsResource.create(...) in PermissionClientTest to ensure the
request reaches authorization logic and still returns Status.FORBIDDEN; update
the assertion to keep expecting Status.FORBIDDEN.getStatusCode().
- Around line 147-153: The test assumes there are at least two default client
scopes by calling clientResource.getDefaultClientScopes().get(1), which can
throw IndexOutOfBoundsException; update PermissionClientTest to first check the
size of clientResource.getDefaultClientScopes() (or pick a safe element such as
index 0 or the first element) before calling get(...), and only call
clientResource.removeDefaultClientScope(clientScopeRep.getId()) when a valid
ClientScopeRepresentation is found (or assert/precondition the list size),
keeping the existing exception assertion around removeDefaultClientScope to
verify ForbiddenException.
🧹 Nitpick comments (4)
server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
304-326: Missing CLIENTS handling ingetResourceNamemethod.The
getResourceNamemethod handles theUSERSresource type to return human-readable names (e.g., "All users" or the username), but there's no equivalent handling for the newCLIENTSresource type. When displaying resource names for client permissions, this will fall through to returningresource.getDisplayName(), which may not be as user-friendly.Consider adding similar handling for CLIENTS to return the client ID or "All clients" for consistency.
♻️ Proposed enhancement
return user.getUsername(); } + } else if (CLIENTS.getType().equals(resourceType)) { + if (resource.getName().equals(CLIENTS_RESOURCE_TYPE)) { + return "All clients"; + } + + ClientModel client = session.clients().getClientById(session.getContext().getRealm(), resource.getName()); + + if (client == null) { + throw new ModelIllegalStateException("Client not found for resource [" + resource.getId() + "]"); + } + + return client.getClientId(); } }services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (2)
224-235: Minor: Consider usingList.of()instead ofArrays.asList()for single-element lists.The code creates a list with a single element using
Arrays.asList(scope). UsingList.of(scope)is more concise and returns an immutable list, which is appropriate here since the list isn't modified.♻️ Proposed improvement
- List<String> expectedScopes = Arrays.asList(scope); + List<String> expectedScopes = List.of(scope);Also applies to: 249-260
262-272: Remove unused methodgetEvaluationContext.The
getEvaluationContextmethod (lines 262-272) is never called within this class or elsewhere in the codebase. Remove it to eliminate dead code.tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java (1)
22-30: Mixing JUnit 4 and JUnit 5 imports.Line 25 imports
org.junit.Assert.fail(JUnit 4), but other assertions use JUnit 5 (org.junit.jupiter.api.Assertions). This inconsistency can cause confusion and potential issues if the test runner changes.Proposed fix
-import static org.junit.Assert.fail; +import static org.junit.jupiter.api.Assertions.fail;
| /** | ||
| * Returns {@code true} if {@link #canView()} returns {@code true}. | ||
| */ | ||
| void requireView(); |
There was a problem hiding this comment.
JavaDoc incorrectly states "Returns true" for a void method.
The JavaDoc for requireView() says "Returns {@code true} if..." but this method has a void return type. Based on the pattern of other requireXxx methods in this interface (e.g., lines 47-50, 59-62), this should say "Throws ForbiddenException if {@link #canView()} returns {@code false}."
📝 Proposed fix
- /**
- * Returns {`@code` true} if {`@link` `#canView`()} returns {`@code` true}.
- */
+ /**
+ * Throws ForbiddenException if {`@link` `#canView`()} returns {`@code` false}.
+ */
void requireView();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Returns {@code true} if {@link #canView()} returns {@code true}. | |
| */ | |
| void requireView(); | |
| /** | |
| * Throws ForbiddenException if {`@link` `#canView`()} returns {`@code` false}. | |
| */ | |
| void requireView(); |
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java`
around lines 95 - 98, Update the JavaDoc for the interface method
ClientPermissionEvaluator.requireView() to stop saying "Returns {`@code` true}"
and instead document that the method throws a ForbiddenException if {`@link`
`#canView`()} returns {`@code` false}; mirror the wording used by other requireXxx
methods (e.g., requireManage(), requireConfigure()) so the comment correctly
reflects the void signature and the thrown exception behavior.
| /** | ||
| * Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}. | ||
| */ | ||
| void requireViewClientScopes(); |
There was a problem hiding this comment.
JavaDoc incorrectly states "Returns true" for a void method.
Similar to requireView(), the JavaDoc for requireViewClientScopes() incorrectly describes the return behavior for a void method.
📝 Proposed fix
- /**
- * Returns {`@code` true} if {`@link` `#canViewClientScopes`()} returns {`@code` true}.
- */
+ /**
+ * Throws ForbiddenException if {`@link` `#canViewClientScopes`()} returns {`@code` false}.
+ */
void requireViewClientScopes();🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java`
around lines 100 - 103, The JavaDoc for requireViewClientScopes() erroneously
says "Returns {`@code` true}" though the method is void; update its Javadoc to
match the semantics used in requireView() (e.g., state that the method enforces
the permission check and will throw an exception if the caller lacks permission
or simply requires view permissions) and remove any "Returns" phrasing so the
comment accurately describes the behavior of requireViewClientScopes().
| try { | ||
| ClientScopeRepresentation clientScopeRep = clientResource.getDefaultClientScopes().get(1); | ||
| clientResource.removeDefaultClientScope(clientScopeRep.getId()); | ||
| fail("Expected exception wasn't thrown."); | ||
| } catch (Exception ex) { | ||
| assertThat(ex, instanceOf(ForbiddenException.class)); | ||
| } |
There was a problem hiding this comment.
Potential IndexOutOfBoundsException when accessing index 1.
getDefaultClientScopes().get(1) assumes at least 2 default client scopes exist. If the test environment has fewer scopes, this will throw an IndexOutOfBoundsException.
Proposed fix
try {
- ClientScopeRepresentation clientScopeRep = clientResource.getDefaultClientScopes().get(1);
+ List<ClientScopeRepresentation> scopes = clientResource.getDefaultClientScopes();
+ if (scopes.isEmpty()) {
+ fail("Expected at least one default client scope");
+ }
+ ClientScopeRepresentation clientScopeRep = scopes.get(0);
clientResource.removeDefaultClientScope(clientScopeRep.getId());
fail("Expected exception wasn't thrown.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| ClientScopeRepresentation clientScopeRep = clientResource.getDefaultClientScopes().get(1); | |
| clientResource.removeDefaultClientScope(clientScopeRep.getId()); | |
| fail("Expected exception wasn't thrown."); | |
| } catch (Exception ex) { | |
| assertThat(ex, instanceOf(ForbiddenException.class)); | |
| } | |
| try { | |
| List<ClientScopeRepresentation> scopes = clientResource.getDefaultClientScopes(); | |
| if (scopes.isEmpty()) { | |
| fail("Expected at least one default client scope"); | |
| } | |
| ClientScopeRepresentation clientScopeRep = scopes.get(0); | |
| clientResource.removeDefaultClientScope(clientScopeRep.getId()); | |
| fail("Expected exception wasn't thrown."); | |
| } catch (Exception ex) { | |
| assertThat(ex, instanceOf(ForbiddenException.class)); | |
| } |
🤖 Prompt for AI Agents
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java`
around lines 147 - 153, The test assumes there are at least two default client
scopes by calling clientResource.getDefaultClientScopes().get(1), which can
throw IndexOutOfBoundsException; update PermissionClientTest to first check the
size of clientResource.getDefaultClientScopes() (or pick a safe element such as
index 0 or the first element) before calling get(...), and only call
clientResource.removeDefaultClientScope(clientScopeRep.getId()) when a valid
ClientScopeRepresentation is found (or assert/precondition the list size),
keeping the existing exception assertion around removeDefaultClientScope to
verify ForbiddenException.
| // can remove a default client scope | ||
| ClientScopeRepresentation clientScopeRep = defaultClientScopes.get(1); | ||
| clientResource.removeDefaultClientScope(clientScopeRep.getId()); | ||
|
|
||
| // can add an optional client scope | ||
| clientResource.addOptionalClientScope(clientScopeRep.getId()); |
There was a problem hiding this comment.
Same hardcoded index issue as above.
Line 174 also uses get(1) which has the same risk. Consider using get(0) or adding a bounds check.
🤖 Prompt for AI Agents
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java`
around lines 173 - 178, The test uses a hardcoded index get(1) on
defaultClientScopes which risks IndexOutOfBounds; change the access to a safe
index or guard it: verify defaultClientScopes is non-empty and select an element
(e.g., defaultClientScopes.get(0)) or add an explicit bounds check before
calling clientResource.removeDefaultClientScope(clientScopeRep.getId()) and
clientResource.addOptionalClientScope(clientScopeRep.getId()) in
PermissionClientTest (referencing defaultClientScopes,
clientResource.removeDefaultClientScope, and
clientResource.addOptionalClientScope).
| // can't create a new client | ||
| try (Response response = realmAdminClient.realm(realm.getName()).clients().create(null)) { | ||
| Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); | ||
| } |
There was a problem hiding this comment.
Passing null to create() may cause unintended server-side errors.
Line 320 passes null as the ClientRepresentation, which could cause a NullPointerException or a 400 Bad Request instead of the expected 403 Forbidden. Use a valid representation to correctly test the authorization check.
Proposed fix
// can't create a new client
- try (Response response = realmAdminClient.realm(realm.getName()).clients().create(null)) {
+ ClientRepresentation newClient = new ClientRepresentation();
+ newClient.setClientId("shouldNotBeCreated");
+ try (Response response = realmAdminClient.realm(realm.getName()).clients().create(newClient)) {
Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // can't create a new client | |
| try (Response response = realmAdminClient.realm(realm.getName()).clients().create(null)) { | |
| Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); | |
| } | |
| // can't create a new client | |
| ClientRepresentation newClient = new ClientRepresentation(); | |
| newClient.setClientId("shouldNotBeCreated"); | |
| try (Response response = realmAdminClient.realm(realm.getName()).clients().create(newClient)) { | |
| Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); | |
| } |
🤖 Prompt for AI Agents
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java`
around lines 319 - 322, The test currently calls
realmAdminClient.realm(realm.getName()).clients().create(null) which can trigger
server-side NPE/400; replace the null with a minimal valid ClientRepresentation
(populate required fields like clientId and any mandatory flags) and use that
when calling ClientsResource.create(...) in PermissionClientTest to ensure the
request reaches authorization logic and still returns Status.FORBIDDEN; update
the assertion to keep expecting Status.FORBIDDEN.getStatusCode().
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Tests